cleanup(bigtable)!: remove experimental InstanceChannelAffinityOption#16213
Conversation
product-auto-label
Bot
added
the
api: bigtable
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the experimental InstanceChannelAffinityOption from the public API, moving it to the internal namespace bigtable_internal, and updates the documentation and tests accordingly. The feedback recommends removing a redundant test case that tests internal implementation details, and correcting a documentation comment in options.h to use the fully qualified name experimental::DynamicChannelPoolSizingPolicyOption.
dbolduc
left a comment
dbolduc
left a comment
There was a problem hiding this comment.
I think this is a cleanup(bigtable)!: remove InstanceChannelAffinityOption
| {InstanceResource(Project(project_id()), instance_id())}, | ||
| std::move(options)); | ||
| } else { | ||
| data_connection_ = MakeDataConnection(std::move(options)); |
There was a problem hiding this comment.
isn't this deprecated? Also, options is empty.
Maybe like:
auto instances = std::vector<InstanceResource>{};
if (GetEnv("GOOGLE_CLOUD_CPP_BIGTABLE_TESTING_CHANNEL_POOL")
.value_or("") == "dynamic") {
instances.push_back(InstanceResource(Project(project_id()), instance_id()));
}
data_connection_ = MakeDataConnection(std::move(instances));Q: is there a downside to always supplying the InstanceResource, even when not using the dynamic channel pool? Then it is just:
data_connection_ = MakeDataConnection(
{InstanceResource(Project(project_id()), instance_id())});There was a problem hiding this comment.
Using the deprecated overload results in a single, static, round-robin channel pool. Using the new overload always uses DynamicChannelPools.
Using the deprecated method allows continued testing of the old channel pool.
| namespace bigtable_internal { | ||
| GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN | ||
|
|
||
| struct InstanceChannelAffinityOption { |
There was a problem hiding this comment.
comment: internal options are a code smell. I am guessing we moved the option to internal to keep the refactor small.
In the long run, it seems ideal for the concrete DataConnectionImpl to hold these std::vector<InstanceResources> as a member.
scotthart
force-pushed
the
bigtable_relocate_instance_option
branch
from
044d1ea to
239c565
Compare
scotthart
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16213 +/- ##
=======================================
Coverage 92.24% 92.24%
=======================================
Files 2265 2265
Lines 210221 210205 -16
=======================================
- Hits 193910 193896 -14
+ Misses 16311 16309 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
With the new
MakeDataConnection(std::vector<InstanceResource>)now available, combining that with the experimentalInstanceChannelAffinityOptionis not intuitive. The experimental option has been removed as the new overload streamlines the instance specification.